-
Notifications
You must be signed in to change notification settings - Fork 0
docs(jsdoc): comprehensive JSDoc pass across all source files #5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Fix misplaced JSDoc blocks in CasService (orphaned restore docs, _chunkAndStore docs attached to _storeChunk), correct broken import path in index.js, and add @param/@returns/@throws/@override/@abstract tags to every public and private method across the codebase.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughPR introduces comprehensive documentation (guides, API references, security model), three runnable example scripts, EventEmitter integration into CasService with lifecycle event emissions, expanded public API (storeFile, restoreFile, createTree, verifyIntegrity), WebCryptoAdapter implementation, enhanced encryption/decryption workflows, and extensive benchmarking and event testing suites. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CAS as ContentAddressableStore<br/>(Facade)
participant Service as CasService<br/>(EventEmitter)
participant Crypto as CryptoAdapter
participant Persistence as GitPersistence
rect rgba(100, 150, 200, 0.5)
Note over User,Persistence: Store & Progress Tracking
User->>CAS: storeFile({filePath, slug, encryptionKey})
CAS->>Service: store(source, slug, encryptionKey)
loop For each chunk
Service->>Service: _storeChunk(chunk, index)
Service->>Crypto: encryptBuffer(chunk, key)
Crypto-->>Service: {buf, meta}
Service->>Persistence: writeBlob(encrypted)
Persistence-->>Service: blobOid
Service->>Service: emit('chunk:stored', {index, digest, ...})
end
Service->>Service: emit('file:stored', {slug, size, chunkCount, encrypted})
Service-->>CAS: Manifest
CAS-->>User: manifest
end
rect rgba(200, 150, 100, 0.5)
Note over User,Persistence: Restore & Verification
User->>CAS: restoreFile({manifest, encryptionKey, outputPath})
CAS->>Service: restore(manifest, encryptionKey)
loop For each chunk
Service->>Persistence: readBlob(blobOid)
Persistence-->>Service: encrypted chunk
Service->>Service: _readAndVerifyChunks (verify digest)
Service->>Service: emit('chunk:restored', {index, ...})
Service->>Crypto: decryptBuffer(chunk, key, meta)
Crypto-->>Service: decrypted chunk
end
Service->>Service: emit('file:restored', {slug, size, chunkCount})
Service-->>CAS: buffer
CAS->>CAS: write to outputPath
CAS-->>User: {bytesWritten}
end
rect rgba(150, 200, 100, 0.5)
Note over Service: Integrity Verification
User->>CAS: verifyIntegrity(manifest)
CAS->>Service: verifyIntegrity(manifest)
alt All chunks match digests
Service->>Service: emit('integrity:pass', {slug})
Service-->>CAS: {valid: true}
else Digest mismatch
Service->>Service: emit('integrity:fail', {slug, chunkIndex, expected, actual})
Service-->>CAS: {valid: false}
end
CAS-->>User: result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Combine M4 Compass changelog/roadmap entries with M5 Sonar + M6 Cartographer. Add JSDoc to new facade methods (readManifest, deleteAsset, findOrphanedChunks).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/domain/errors/CasError.js (1)
18-18:⚠️ Potential issue | 🔴 Critical
Error.captureStackTraceis V8-specific and will throw in non-V8 runtimes.The project supports Deno and Web environments (via
WebCryptoAdapter), whereError.captureStackTracemay not exist. This line is called unconditionally and will throw aTypeErrorat runtime in those environments.Proposed fix
- Error.captureStackTrace(this, this.constructor); + if (typeof Error.captureStackTrace === 'function') { + Error.captureStackTrace(this, this.constructor); + }src/infrastructure/adapters/NodeCryptoAdapter.js (1)
33-39:⚠️ Potential issue | 🟡 Minor
decryptBufferskips#validateKey— inconsistent withencryptBuffer.
encryptBuffer(line 21) callsthis.#validateKey(key), butdecryptBufferdoes not. Passing an invalid key here will surface a rawnode:cryptoerror instead of a descriptiveCasErrorwithINVALID_KEY_TYPE/INVALID_KEY_LENGTH. This is pre-existing behavior, but since this PR is the comprehensive JSDoc pass touching these methods, it's a good time to address the gap.🛡️ Proposed fix
/** `@override` */ decryptBuffer(buffer, key, meta) { + this.#validateKey(key); const nonce = Buffer.from(meta.nonce, 'base64');src/domain/value-objects/Manifest.js (1)
22-37:⚠️ Potential issue | 🟠 MajorDiscarded
ManifestSchema.parse()result causes theencrypteddefault to be silently lost.
ManifestSchema.parse(data)is called for validation only — the return value (which carries Zod defaults and transforms) is discarded. This matters becauseEncryptionSchemadefinesencrypted: z.boolean().default(true). If a caller passes encryption metadata without theencryptedfield, the schema parse succeeds (applying the default), but{ ...data.encryption }on line 29 copies the raw input, which lacksencrypted.🐛 Proposed fix — use the parsed result
try { - ManifestSchema.parse(data); - this.slug = data.slug; - this.filename = data.filename; - this.size = data.size; - this.chunks = data.chunks.map((c) => new Chunk(c)); - this.encryption = data.encryption ? { ...data.encryption } : undefined; + const validated = ManifestSchema.parse(data); + this.slug = validated.slug; + this.filename = validated.filename; + this.size = validated.size; + this.chunks = validated.chunks.map((c) => new Chunk(c)); + this.encryption = validated.encryption ? { ...validated.encryption } : undefined; Object.freeze(this);src/infrastructure/adapters/WebCryptoAdapter.js (1)
109-113:⚠️ Potential issue | 🟠 Major
finalize()will crash with a nulltagif called before the encrypt generator completes.
finalTagis initialized tonull(line 83) and only set after the one-shot encrypt completes (line 104). Iffinalize()is called prematurely,this.#buildMeta(nonce, null)→this.#toBase64(null)throws.BunCryptoAdapterguards against this with astreamFinalizedflag — add the same guard here.🛡️ Proposed fix
const chunks = []; let finalTag = null; + let streamFinalized = false; const encrypt = async function* (source) { // ... finalTag = fullBuffer.slice(-tagLength); + streamFinalized = true; yield Buffer.from(ciphertext); }; const finalize = () => { + if (!streamFinalized) { + throw new CasError( + 'Cannot finalize before the encrypt stream is fully consumed', + 'STREAM_NOT_CONSUMED', + ); + } return this.#buildMeta(nonce, finalTag); };index.js (1)
213-221:⚠️ Potential issue | 🟠 Major
writeFileSyncblocks the event loop — usewriteFile(async) for a library API.
restoreFileis anasyncmethod, so callers expect it to be non-blocking. UsingwriteFileSyncfor potentially large restored files (10 MB+ per your benchmarks) will stall the event loop. Switch to the asyncfs.promises.writeFile.Proposed fix
-import { createReadStream, writeFileSync } from 'node:fs'; +import { createReadStream } from 'node:fs'; +import { writeFile } from 'node:fs/promises'; import path from 'node:path';async restoreFile({ manifest, encryptionKey, outputPath }) { const service = await this.#getService(); const { buffer, bytesWritten } = await service.restore({ manifest, encryptionKey, }); - writeFileSync(outputPath, buffer); + await writeFile(outputPath, buffer); return { bytesWritten }; }src/domain/services/CasService.js (1)
273-275:⚠️ Potential issue | 🟡 Minor
restoreskips thefile:restoredevent for empty manifests.When
manifest.chunks.length === 0, the method returns early (line 274) without emittingfile:restored. Consumers listening for this event to track all restore operations will miss empty-file restores, breaking the documented contract thatfile:restoredfires once per restore.Proposed fix
if (manifest.chunks.length === 0) { + this.emit('file:restored', { + slug: manifest.slug, size: 0, chunkCount: 0, + }); return { buffer: Buffer.alloc(0), bytesWritten: 0 }; }
🤖 Fix all issues with AI agents
In `@docs/SECURITY.md`:
- Line 63: Update the SECURITY.md line that claims the algorithm is "via Node.js
`node:crypto` module" to reflect multi-runtime support: state that the
underlying algorithm is AES-256-GCM (aes-256-gcm) and that runtime-specific
adapters (e.g., BunCryptoAdapter and WebCryptoAdapter, in addition to the
Node.js crypto adapter) implement that algorithm for Bun, Deno/Web, and Node.js
respectively; mention that the concrete provider varies by runtime and point to
those adapters as the implementations.
In `@examples/encrypted-workflow.js`:
- Around line 66-78: The manifest encryption fields used are incorrect: replace
references to manifest.encryption.iv and manifest.encryption.authTag with the
actual fields produced by NodeCryptoAdapter.#buildMeta
(manifest.encryption.nonce and manifest.encryption.tag), decode those base64
strings to Buffers to compute lengths and to produce the hex dump (e.g.,
Buffer.from(manifest.encryption.nonce, 'base64') and
Buffer.from(manifest.encryption.tag, 'base64')), keep algorithm fallback to
'AES-256-GCM' and preserve the error exit branch when
manifest.encryption.encrypted is falsy.
In `@examples/progress-tracking.js`:
- Around line 200-210: The summary is wrong because progress.store (and
progress.store.chunks/bytes) keeps accumulating across both store steps due to
the persistent "chunk:stored" listener; fix by either unregistering that
listener before Step 4 (call the emitter's off/removeListener for the same
handler used when subscribing) or by snapshotting the Step 1 totals immediately
after Step 1 completes (e.g., store storeChunksStep1 = progress.store.chunks and
storeBytesStep1 = progress.store.bytes) and then use those snapshot variables in
the summary/throughput calculation instead of progress.store.*; locate the
"chunk:stored" listener registration and the summary block that reads
progress.store to implement one of these fixes.
In `@GUIDE.md`:
- Around line 388-393: The example incorrectly imports writeFileSync from
'node:crypto'; change the import so randomBytes stays imported from
'node:crypto' and writeFileSync is imported from 'node:fs' (i.e., use
randomBytes and writeFileSync with correct module imports) so generating the
32-byte key with randomBytes(32) and writing it to './vacation.key' succeeds
without ImportError.
In `@ROADMAP.md`:
- Around line 135-136: The Markdown table rows for "v1.5.0" and "v1.6.0" have an
extra cell (the trailing ✅) causing a column count mismatch; fix by either
merging the checkmark into the existing "Theme" cell for those rows (so the
Theme column for v1.5.0 and v1.6.0 becomes e.g. "Observability ✅" and
"Documentation ✅") or by adding a new "Status" column to the table header and
moving each trailing ✅ into that new "Status" column so the header and all rows
have the same number of cells.
In `@src/infrastructure/adapters/WebCryptoAdapter.js`:
- Around line 11-23: The WebCryptoAdapter.randomBytes implementation returns a
Uint8Array but the CryptoPort.randomBytes contract expects a Buffer; update
WebCryptoAdapter.randomBytes to detect if Buffer exists on globalThis and, when
available, convert the Uint8Array into a Buffer (otherwise continue returning
the Uint8Array) so callers that rely on Buffer methods (e.g., .toString('hex'))
won't break; locate the randomBytes method inside class WebCryptoAdapter and
ensure behavior matches CryptoPort.randomBytes' documented return type while
preserving compatibility with environments that only accept Uint8Array.
In `@test/benchmark/cas.bench.js`:
- Around line 160-188: The decrypt benchmarks pass enc.buffer which is undefined
because CasService.encrypt() (and CryptoPort.encryptBuffer()) returns { buf,
meta }; update all benchmark calls to service.decrypt to pass enc.buf (not
enc.buffer) — specifically in the decrypt blocks for 'decrypt – 1KB', 'decrypt –
1MB' and 'decrypt – 10MB' replace usages of enc.buffer with enc.buf so decrypt
receives the actual encrypted bytes and meta remains unchanged.
🧹 Nitpick comments (8)
src/domain/value-objects/Chunk.js (1)
25-28: Consider using the return value ofChunkSchema.parse().
ChunkSchema.parse(data)returns the validated (and potentially transformed) data, but the result is discarded. If a Zod transform or default is ever added toChunkSchema,Object.assign(this, data)would silently use the raw input instead of the parsed output.♻️ Suggested improvement
- ChunkSchema.parse(data); - Object.assign(this, data); + const validated = ChunkSchema.parse(data); + Object.assign(this, validated);GUIDE.md (1)
347-364: Inconsistent import paths for internal modules across examples.Line 359 uses a deep source path (
@git-stunts/git-cas/src/domain/value-objects/Manifest.js), while line 853 uses a subpath export (@git-stunts/git-cas/service). Deep source paths are fragile and may break if the internal directory structure changes. Consider using consistent public API imports or subpath exports throughout the guide, or add a note that these are internal paths.src/infrastructure/adapters/BunCryptoAdapter.js (2)
40-48:decryptBuffervalidates key here butNodeCryptoAdapter.decryptBufferdoes not.
BunCryptoAdapter.decryptBuffercalls#validateKey(key)(line 42), whileNodeCryptoAdapter.decryptBuffer(seeNodeCryptoAdapter.jslines 33–39) skips validation. SinceCasService.decryptdoesn't call_validateKeybefore delegating, an invalid key passed toNodeCryptoAdapterwill produce a raw Node crypto error rather than a cleanCasError. Consider adding#validateKeytoNodeCryptoAdapter.decryptBufferas well for consistency.
50-83:createEncryptionStreamhas astreamFinalizedguard — nice, but missing fromNodeCryptoAdapter.The
STREAM_NOT_CONSUMEDcheck (lines 72–77) is a useful safeguard.NodeCryptoAdapter.createEncryptionStreamlacks this guard and will callcipher.getAuthTag()beforecipher.final()iffinalize()is invoked prematurely — resulting in a cryptic Node error. Consider porting this guard.src/infrastructure/adapters/WebCryptoAdapter.js (1)
49-70:decryptBufferdoes not validate the key — inconsistent withencryptBuffer.
encryptBuffercalls#validateKey(line 27), butdecryptBufferdoes not. An invalid key will produce a cryptic Web Crypto error instead of a cleanCasError. This matches the same gap inNodeCryptoAdapter. Consider addingthis.#validateKey(key)at the top ofdecryptBufferfor defensive consistency.Proposed fix
async decryptBuffer(buffer, key, meta) { + this.#validateKey(key); const nonce = this.#fromBase64(meta.nonce);index.js (1)
58-68: Redundant initialization:this.#servicePromise = nullin constructor duplicates the field declaration.Line 65 sets
this.#servicePromise = null, but the private field declaration at line 68 already initializes it tonullbefore the constructor body runs. The assignment in the constructor is a no-op.Proposed fix — remove redundant assignment
constructor({ plumbing, chunkSize, codec, policy, crypto }) { this.plumbing = plumbing; this.chunkSizeConfig = chunkSize; this.codecConfig = codec; this.policyConfig = policy; this.cryptoConfig = crypto; this.service = null; - this.#servicePromise = null; }src/domain/services/CasService.js (1)
145-155:decryptdoes not validate the key before delegating to the crypto port.
encrypt(line 132) calls_validateKey, butdecryptdoes not. If called directly (outside ofrestore), an invalid key will produce a raw crypto error instead ofCasError('INVALID_KEY_TYPE'). Consider adding_validateKeywhenmeta?.encryptedis truthy, for symmetry withencrypt.Proposed fix
async decrypt({ buffer, key, meta }) { if (!meta?.encrypted) { return buffer; } + this._validateKey(key); try { return await this.crypto.decryptBuffer(buffer, key, meta);examples/progress-tracking.js (1)
170-180: Hardcoded chunk size in progress estimation.Line 175 uses the literal
128 * 1024to estimate total chunks instead ofcas.chunkSize, duplicating the value from line 30. If the chunk size on line 30 changes, this estimate silently becomes wrong.♻️ Proposed fix
- totalChunks = Math.ceil(fileSize / (128 * 1024)); + totalChunks = Math.ceil(fileSize / cas.chunkSize);
- SECURITY.md: generalize crypto description for multi-runtime support - encrypted-workflow.js: fix field names (nonce/tag, not iv/authTag) - progress-tracking.js: snapshot Step 1 counters to avoid accumulation - GUIDE.md: fix writeFileSync import (node:fs, not node:crypto) - ROADMAP.md: add Status column header to fix table column mismatch - WebCryptoAdapter: wrap randomBytes return in Buffer when available - cas.bench.js: fix enc.buffer → enc.buf in all decrypt benchmarks
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary
_chunkAndStoredocs on_storeChunk, orphanedrestoredocs above_readAndVerifyChunks), broken@paramimport path inindex.js@param/@returns/@throws/@override/@abstracttags to every public and private method across all 17 source filesTest plan
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests